Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

dnsdist: add EDNSOptionRule #6803

Merged
merged 3 commits into from Aug 8, 2018
Merged

dnsdist: add EDNSOptionRule #6803

merged 3 commits into from Aug 8, 2018

Conversation

Habbie
Copy link
Member

@Habbie Habbie commented Jul 24, 2018 •
edited

Short description

This adds EDNSOptionRule, used as follows (8 is edns-client-subnet):

warnlog(string.format("Script starting %s", "up!"))

newServer({address="8.8.8.8", pool="g1"})
newServer({address="8.8.4.4", pool="g2"})

addAction(EDNSOptionRule(8), PoolAction("g1"))
addAction(AllRule(), PoolAction("g2"))

getServer(0):setUp()
getServer(1):setUp()

Needs:

  • tests
  • documentation
  • a good review of the pointer mangling that I cargo culted from another function in dnsdist-ecs.cc

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

zeha
zeha reviewed Jul 26, 2018
View reviewed changes
pdns/dnsdist-ecs.cc Outdated
bool isEDNSOptionInOpt(const char* optStart, const size_t optLen, const uint16_t optionCodeToFind)
{
/* we need at least:
root label (1), type (2), class (2), ttl (4) + rdlen (2)*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've seen this comment so often already, maybe it's time to stick the lengths into a #define or think about some other code sharing...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, at least dnsdist-ecs.cc might need some refactoring.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but let's do that after this PR?

@rgacogne rgacogne added this to the dnsdist-1.3.x milestone Jul 30, 2018
rgacogne
rgacogne approved these changes Jul 30, 2018
View reviewed changes
Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, a "negative" test would be nice.

regression-tests.dnsdist/test_Advanced.py Outdated
addAction(EDNSOptionRule(8), DropAction())
"""

def testDropped(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to add a test proving that a query without the corresponding option is not dropped :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushed

regression-tests.dnsdist/test_Advanced.py Outdated

_config_template = """
newServer{address="127.0.0.1:%s"}
addAction(EDNSOptionRule(8), DropAction())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to export EDNSOptionCode.Cookie, EDNSOptionCode.ECS and so on to Lua.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushed

pdns/dnsdist-ecs.cc Outdated
bool isEDNSOptionInOpt(const char* optStart, const size_t optLen, const uint16_t optionCodeToFind)
{
/* we need at least:
root label (1), type (2), class (2), ttl (4) + rdlen (2)*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, at least dnsdist-ecs.cc might need some refactoring.

@rgacogne rgacogne modified the milestones: dnsdist-1.3.x, dnsdist-1.4.0 Jul 30, 2018
rgacogne
rgacogne approved these changes Aug 7, 2018
View reviewed changes
Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Habbie Habbie changed the title [WIP] dnsdist: add EDNSOptionRule dnsdist: add EDNSOptionRule Aug 7, 2018
@rgacogne
Copy link
Member

rgacogne commented Aug 8, 2018

This PR now has a conflict following my merge of #6831, sorry about that :-/

@Habbie
Copy link
Member Author

Habbie commented Aug 8, 2018

This PR now has a conflict following my merge of #6831, sorry about that :-/

Rebased and force pushed; switched to string like the rest of #6831 so the code is better for it!

Please re-review; I'll squash after.

@Habbie
Copy link
Member Author

Habbie commented Aug 8, 2018

Squashed!

rgacogne
rgacogne reviewed Aug 8, 2018
View reviewed changes
pdns/dnsdist-lua-vars.cc Outdated
@@ -69,6 +69,20 @@ void setupLuaVars()
{"Additional",3 }
});

g_lua.writeVariable("EDNSOptionCode", std::unordered_map<string,int>{
{"NSID", 3 },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit, but perhaps this would be better:

{ "NSID",          EDNSOptionCode::NSID },
...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushed

rgacogne
rgacogne approved these changes Aug 8, 2018
View reviewed changes
Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rgacogne rgacogne merged commit 64c0fe7 into PowerDNS:master Aug 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zeha zeha zeha left review comments

@rgacogne rgacogne rgacogne approved these changes

Assignees
No one assigned
Labels
dnsdist enhancement
Projects
None yet
Milestone
dnsdist-1.4.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Habbie @rgacogne @zeha